Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extend xportr_write to accept metadata and deprecate label #185

Merged
merged 13 commits into from
Dec 7, 2023
Merged

Extend xportr_write to accept metadata and deprecate label #185

merged 13 commits into from
Dec 7, 2023

Conversation

vedhav
Copy link
Collaborator

@vedhav vedhav commented Nov 16, 2023 •
edited
Loading

Changes Description

The xportr_write() function follows the same design as the rest of the xportr_* functions by asking for the metadata argument. Thus we're deprecating the label argument from this function.

Setting the dataset label can be done either using xportr_write() or xportr_df_label()

Here is some code to test the xportr_write()

devtools::load_all()

metadata <- data.frame(
  dataset = c("adsl", "adae"),
  label = c("Subject-Level Analysis", "Adverse Events Analysis")
)
adsl <- data.frame(
  USUBJID = c(1001, 1002, 1003),
  SITEID = c(001, 002, 003),
  AGE = c(63, 35, 27),
  SEX = c("M", "F", "M")
)

# Writing with no metadata preset and no metadata passed to `xportr_write()`
adsl |>
  xportr_write(
    path = paste0("nono.xpt"),
    strict_checks = TRUE
  )

# Writing with no metadata preset and passing `label` to `xportr_write()` which will be deprecated
adsl |>
  xportr_write(
    path = paste0("noyes1.xpt"),
    strict_checks = TRUE,
    label = "Subject-Level Analysis"
  )
# Writing with no metadata preset and passing `metadata` to `xportr_write()`
adsl |>
  xportr_write(
    path = paste0("noyes2.xpt"),
    strict_checks = TRUE,
    metadata = metadata
  )

adsl <- xportr_df_label(adsl, metadata)
# Writing with metadata preset and no metadata passed to `xportr_write()`
adsl |>
  xportr_write(
    path = paste0("yesno.xpt"),
    strict_checks = TRUE
  )


# Writing with metadata preset and passing `label` to `xportr_write()` which will be deprecated
adsl |>
  xportr_write(
    path = paste0("yesyes1.xpt"),
    strict_checks = TRUE,
    label = "Subject-Level Analysis"
  )
# Writing with metadata preset and passing `metadata` to `xportr_write()`
adsl |>
  xportr_write(
    path = paste0("yesyes2.xpt"),
    strict_checks = TRUE,
    metadata = metadata
  )

nono <- read_xpt("nono.xpt")
noyes1 <- read_xpt("noyes1.xpt")
noyes2 <- read_xpt("noyes2.xpt")
yesno <- read_xpt("yesno.xpt")
yesyes1 <- read_xpt("yesyes1.xpt")
yesyes2 <- read_xpt("yesyes2.xpt")

attr(nono, "label")
# NULL
attr(noyes1, "label")
# [1] "Subject-Level Analysis"
attr(noyes2, "label")
# [1] "Subject-Level Analysis"
attr(yesno, "label")
# [1] "Subject-Level Analysis"
attr(yesyes1, "label")
# [1] "Subject-Level Analysis"
attr(yesyes2, "label")
# [1] "Subject-Level Analysis"

Task List

  • The spirit of xportr is met in your Pull Request
  • Place Closes Site examples not using xportr_df_label() #179 into the beginning of your Pull Request Title (Use Edit button in top-right if you need to update)
  • Summary of changes filled out in the above Changes Description. Can be removed or left blank if changes are minor/self-explanatory.
  • Check that your Pull Request is targeting the devel branch, Pull Requests to main should use the Release Pull Request Template
  • Code is formatted according to the tidyverse style guide. Use styler package and functions to style files accordingly.
  • Updated relevant unit tests or have written new unit tests. See our Wiki for conventions used in this package. Still at 100% ;) But need to update the tests.
  • Creation/updated relevant roxygen headers and examples. See our Wiki for conventions used in this package.
  • Run devtools::document() so all .Rd files in the man folder and the NAMESPACE file in the project root are updated appropriately
  • Run pkgdown::build_site() and check that all affected examples are displayed correctly and that all new/updated functions occur on the "Reference" page.
  • Update NEWS.md if the changes pertain to a user-facing function (i.e. it has an @export tag) or documentation aimed at users (rather than developers)
  • Address any updates needed for vignettes and/or templates
  • Link the issue Development Panel so that it closes after successful merging.
  • Fix merge conflicts
  • Pat yourself on the back for a job well done! Much love to your accomplishment!

@vedhav vedhav linked an issue Nov 16, 2023 that may be closed by this pull request
Site examples not using xportr_df_label() #179
Closed
@bms63 bms63 changed the base branch from devel to main November 16, 2023 13:28
@codecov Codecov
Copy link

codecov bot commented Nov 21, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9725e9a) 100.00% compared to head (3d90cd0) 100.00%.
Report is 1 commits behind head on main.

❗ Current head 3d90cd0 differs from pull request most recent head 8437f07. Consider uploading reports for the commit 8437f07 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff            @@
##              main      #185   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines          581       588    +7     
=========================================
+ Hits           581       588    +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

elimillera
elimillera approved these changes Nov 27, 2023
View reviewed changes
Copy link
Member

@elimillera elimillera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple small doc comments but looks great

NEWS.md Outdated Show resolved Hide resolved
R/write.R Outdated Show resolved Hide resolved
vedhav and others added 3 commits November 28, 2023 03:44
@vedhav @elimillera
Update NEWS.md
d75aefa 
Co-authored-by: Eli Miller <elimillera@gmail.com>
@vedhav @elimillera
Update R/write.R
c6725b1 
Co-authored-by: Eli Miller <elimillera@gmail.com>
@vedhav
docs: update roxygen docs
25c7bc4
bms63
bms63 reviewed Nov 29, 2023
View reviewed changes
README.Rmd Show resolved Hide resolved
bms63
bms63 reviewed Nov 29, 2023
View reviewed changes
Copy link
Collaborator

@bms63 bms63 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I just have concerns on the documentation when we don't use xportr_metadata() to set everything for the dataset label.

@vedhav
Copy link
Collaborator Author

vedhav commented Dec 5, 2023

Not sure if I went overboard by exporting the dataset_spec so asking for a re-review. Perhaps just reading it from the path system.file(paste0("specs/", "ADaM_admiral_spec.xlsx"), package = "xportr") could have been okay, like we do in the README.md What are your thoughts?

@vedhav vedhav merged commit ef3170b into main Dec 7, 2023
11 checks passed
@vedhav vedhav deleted the 179_fetch_dataset_label_from_metadata branch December 7, 2023 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bms63 bms63 Awaiting requested review from bms63

@elimillera elimillera Awaiting requested review from elimillera

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Site examples not using xportr_df_label()
3 participants
@vedhav @bms63 @elimillera